fix(net): authorize governance inv responses via the net-layer per-peer request tracker - #7442
Conversation
|
✅ Review complete (commit 0c0ec8c) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd5d721e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const bool announced = state->m_object_download.m_object_announced.erase(inv) != 0; | ||
| const bool in_flight = state->m_object_download.m_object_in_flight.erase(inv) != 0; |
There was a problem hiding this comment.
Clear queued process entries when consuming announcements
When authorization is satisfied by a bare announcement, this erases m_object_announced but leaves the same inv queued in m_object_process_time. For governance votes/objects that are not recorded as already-have after processing (for example orphan votes or invalid objects), SendMessages() later drains that stale queue entry and sends a redundant GETDATA back to the same peer; because this new path deliberately avoids g_erased_object_requests, the old skip no longer prevents that retry. The receive-side consume path should also remove queued process-time entries for the inv, or only leave in-flight requests eligible for later handling.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between c89ec5272c99f0b274f119ac35ad72e3042a1898 and 0c0ec8c. 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughGovernance inventory request tracking is removed from Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetGovernance
participant PeerManager
participant CGovernanceManager
Peer->>NetGovernance: Send governance object or vote
NetGovernance->>PeerManager: Consume per-peer object request
PeerManager-->>NetGovernance: Authorization result
NetGovernance->>CGovernanceManager: Process accepted message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/net_processing.h (1)
67-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the consuming side effect and add the
cs_mainannotation toPeerRequestedObject.
PeerRequestedObject's doc comment reads as a pure query, but the implementation (RequestedObjectin net_processing.cpp) erases the per-peerm_object_announced/m_object_in_flightentry as a side effect — unlike its siblingPeerEraseObjectRequest, whose comment explicitly calls out "update global request tracking." A caller could reasonably assume this method is idempotent/non-mutating.Separately, the concrete override carries
EXCLUSIVE_LOCKS_REQUIRED(::cs_main), but this pure-virtual declaration doesn't. SinceNetGovernance(and otherNetHandlers) only see this through thePeerManagerInternal*base pointer, Clang thread-safety analysis at those call sites is governed by this interface declaration, not the override — so thecs_mainprecondition isn't statically enforced where it matters most.📝 Suggested fix
/** Erase a pending object request for a peer and update global request tracking. */ virtual void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) = 0; - /** Return whether the peer announced this inv or we requested it from the peer. */ - virtual bool PeerRequestedObject(NodeId nodeid, const CInv& inv) = 0; + /** Return whether the peer announced this inv or we requested it from the peer. + * Consumes (erases) the matching per-peer announced/in-flight entry as a side effect. */ + virtual bool PeerRequestedObject(NodeId nodeid, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/net_processing.h` around lines 67 - 70, Update the PeerRequestedObject declaration in PeerManagerInternal to document that it may erase the peer’s announced/in-flight object state and update request tracking, rather than presenting it as a pure query. Add EXCLUSIVE_LOCKS_REQUIRED(::cs_main) to the pure-virtual declaration so callers through the interface are checked for the required lock, matching the concrete RequestedObject override.src/test/governance_inv_tests.cpp (1)
334-390: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a tracker-consumption assertion for
second_announcing_peer.The vote test (line 436) verifies
second_announcing_peer's per-peer request entry is consumed after acceptance; the object test doesn't do the analogous check for itssecond_announcing_peer(376-378). Adding it would guard against a regression where consumption is accidentally shared/global instead of per-peer.✅ Suggested addition
ProcessGovernanceObject(net_gov, *second_announcing_peer, govobj); + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerRequestedObject(second_announcing_peer->GetId(), object_inv))); BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(second_announcing_peer->GetId(), stats));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/governance_inv_tests.cpp` around lines 334 - 390, Add an assertion immediately after processing the object for second_announcing_peer in governance_objects_require_peer_announcement_or_request, verifying PeerRequestedObject for that peer and object_inv is false. Keep the existing misbehavior checks so the test confirms per-peer tracker consumption independently for both announcing peers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/net_processing.h`:
- Around line 67-70: Update the PeerRequestedObject declaration in
PeerManagerInternal to document that it may erase the peer’s announced/in-flight
object state and update request tracking, rather than presenting it as a pure
query. Add EXCLUSIVE_LOCKS_REQUIRED(::cs_main) to the pure-virtual declaration
so callers through the interface are checked for the required lock, matching the
concrete RequestedObject override.
In `@src/test/governance_inv_tests.cpp`:
- Around line 334-390: Add an assertion immediately after processing the object
for second_announcing_peer in
governance_objects_require_peer_announcement_or_request, verifying
PeerRequestedObject for that peer and object_inv is false. Keep the existing
misbehavior checks so the test confirms per-peer tracker consumption
independently for both announcing peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb58df3f-d103-4407-b0ef-364cc46aeb3b
📥 Commits
Reviewing files that changed from the base of the PR and between 2bbf4a4 and 3dd5d721e96bd1f3d6c50a0575162ecf0683706f.
📒 Files selected for processing (8)
src/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/init.cppsrc/net_processing.cppsrc/net_processing.hsrc/test/governance_inv_tests.cppsrc/test/net_tests.cpp
💤 Files with no reviewable changes (1)
- src/governance/governance.h
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.6-sol(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.6-sol(failed); verifier=opus.
Carried-forward prior findings: None. The prior anchor 2bbf4a4 is not a prior reviewed PR head, so prior-finding classification is not applicable and no finding is carried forward; resolved_findings is empty. New findings in latest delta: Two verified non-blocking observations on the governance request-authorization refactor. This PR removes the unbounded governance-owned request cache and authorizes MNGOVERNANCEOBJECT/VOTE responses via the already-bounded per-peer net-layer tracker through the new PeerRequestedObject. The design is sound and no dangling references to removed symbols remain. Both findings are non-blocking (one suggestion, one nitpick), so the review action is COMMENT.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/net_processing.cpp`:
- [SUGGESTION] src/net_processing.cpp:1592-1604: Invalid governance objects announced by multiple peers are no longer suppressed from re-download
The old receive path called PeerEraseObjectRequest -> EraseObjectRequest, which sets g_erased_object_requests[hash] (net_processing.cpp:1586). In SendMessages the scheduler skips any inv present in g_erased_object_requests (net_processing.cpp:6688), so once an object was received from one peer, the same hash queued from other announcers was not re-requested for the limitedmap lifetime. The new RequestedObject (net_processing.cpp:1601-1603) erases only the per-peer m_object_announced/m_object_in_flight and intentionally does NOT touch the global markers.
For objects that end up stored this is harmless because AlreadyHave suppresses re-download. But ProcessObject returns false and stores nothing for a genuinely invalid object, after which the peer is penalized +20 (net_governance.cpp:190-192). With the global marker no longer set, the same invalid hash announced by other peers (or re-announced after the per-peer tracker is cleared) will be re-requested, re-downloaded over a full getdata round-trip, and re-validated. This is self-limiting per peer (ban at 100) and only affects invalid-object spam across distinct peers on a best-effort gossip path, so it is not a correctness bug — but it is a real reduction in the DoS-resistance the removed global marker provided on this path. Confirm this tradeoff is acceptable versus still recording a lightweight erased marker on receipt.
| bool PeerManagerImpl::RequestedObject(NodeId nodeid, const CInv& inv) | ||
| { | ||
| AssertLockHeld(cs_main); | ||
|
|
||
| CNodeState* state = State(nodeid); | ||
| if (state == nullptr) | ||
| return false; | ||
|
|
||
| LogPrint(BCLog::NET, "%s -- inv=(%s)\n", __func__, inv.ToString()); | ||
| const bool announced = state->m_object_download.m_object_announced.erase(inv) != 0; | ||
| const bool in_flight = state->m_object_download.m_object_in_flight.erase(inv) != 0; | ||
| return announced || in_flight; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Invalid governance objects announced by multiple peers are no longer suppressed from re-download
The old receive path called PeerEraseObjectRequest -> EraseObjectRequest, which sets g_erased_object_requests[hash] (net_processing.cpp:1586). In SendMessages the scheduler skips any inv present in g_erased_object_requests (net_processing.cpp:6688), so once an object was received from one peer, the same hash queued from other announcers was not re-requested for the limitedmap lifetime. The new RequestedObject (net_processing.cpp:1601-1603) erases only the per-peer m_object_announced/m_object_in_flight and intentionally does NOT touch the global markers.
For objects that end up stored this is harmless because AlreadyHave suppresses re-download. But ProcessObject returns false and stores nothing for a genuinely invalid object, after which the peer is penalized +20 (net_governance.cpp:190-192). With the global marker no longer set, the same invalid hash announced by other peers (or re-announced after the per-peer tracker is cleared) will be re-requested, re-downloaded over a full getdata round-trip, and re-validated. This is self-limiting per peer (ban at 100) and only affects invalid-object spam across distinct peers on a best-effort gossip path, so it is not a correctness bug — but it is a real reduction in the DoS-resistance the removed global marker provided on this path. Confirm this tradeoff is acceptable versus still recording a lightweight erased marker on receipt.
source: ['claude']
| /** Return whether the peer announced this inv or we requested it from the peer. */ | ||
| virtual bool PeerRequestedObject(NodeId nodeid, const CInv& inv) = 0; |
There was a problem hiding this comment.
💬 Nitpick: PeerRequestedObject reads as a pure predicate but consumes per-peer state
PeerRequestedObject/RequestedObject returns bool and its name reads as a query ('did the peer request this?'), but it also erases the inv from m_object_announced and m_object_in_flight as a side effect (net_processing.cpp:1601-1602). The consume-once semantics are load-bearing and tested (net_tests.cpp:95-96 assert the second call returns false), but a boolean-named function that mutates state is easy to misuse at future call sites — calling it to test membership would silently consume the entry. The header comment (net_processing.h:69) documents only the return value, not the erase side effect. Consider a name that conveys consume-and-check semantics (e.g. ConsumePeerObjectRequest) or documenting the mutation in the interface comment.
source: ['claude']
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c89ec5272c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (auto it = process_time.begin(); it != process_time.end();) { | ||
| const bool match{it->second.type == inv.type && it->second.hash == inv.hash}; | ||
| it = match ? process_time.erase(it) : std::next(it); |
There was a problem hiding this comment.
Avoid quadratic scans when consuming announced objects
When a peer first sends a large INV of governance vote hashes and then pushes the matching MNGOVERNANCEOBJECTVOTE messages before we send GETDATA, every accepted push enters this loop and scans the entire remaining m_object_process_time while message processing holds cs_main. Unknown-parent governance votes are stored as orphans without a peer penalty, so one peer can queue up to MAX_INV_SZ entries and force O(n²) iterator work just by ordering the pushed votes adversarially. Please avoid a full multimap scan on each consume (or add indexed removal/bounded cleanup) so consuming queued announcements stays proportional to the item being removed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.6-sol(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.6-sol(failed); verifier=opus.
The current-head implementation fixes the consume-once API naming/documentation issue and correctly removes the consumed inv from this peer's queued request schedule. The earlier cross-peer invalid-object re-download tradeoff remains, and the latest review-fix commit should be folded into the commits whose unshipped code it corrects. No blocking issues.
🟡 2 suggestion(s)
Prior Finding Reconciliation
- STILL VALID — Invalid governance objects announced by multiple peers are no longer suppressed from re-download.
ConsumeObjectRequestremoves only per-peer announced/in-flight/queued state and deliberately leavesg_erased_object_requestsuntouched. An invalid hash that is not stored can therefore still be requested, downloaded, and revalidated from other announcing peers. This remains a low-confidence, non-blocking DoS-resistance tradeoff. - FIXED — PeerRequestedObject reads as a pure predicate but consumes per-peer state. The API is now named
PeerConsumeObjectRequest/ConsumeObjectRequest, andsrc/net_processing.h:69-72explicitly documents that it erases announced, in-flight, and queued request state.
Carried-Forward Prior Findings
- [SUGGESTION]
src/net_processing.cpp:1604-1615— Invalid governance objects announced by multiple peers are no longer suppressed from re-download. The old receive path recordedg_erased_object_requests[hash], which madeSendMessagessuppress subsequent requests for that hash during the marker lifetime. The current per-peer consume path intentionally does not set that global marker. Stored objects are protected byAlreadyHave, but a genuinely invalid object is not stored, so distinct peers can cause another full GETDATA and validation pass before each peer's +20 misbehavior penalties become self-limiting. Please confirm that this tradeoff is acceptable versus recording a lightweight marker only after local validation failure.
New Findings In Latest Delta
- [SUGGESTION]
c89ec5272c9— Squash “fix: apply review suggestions” into the commits it corrects. This commit only corrects code introduced earlier in this PR: it renames the new consume API, fixes the new consume path to purgem_object_process_time, and updates the tests added by the preceding test commit. Dash merges PR commits without squashing, so the generic review-context-dependent commit would become permanent bisect/blame noise. Fold the production changes into94d976989b2and the test changes into3dd5d721e96before merge.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] src/net_processing.cpp:1604-1615: Invalid governance objects announced by multiple peers are no longer suppressed from re-download. The per-peer consume path deliberately leaves the global erased-object marker untouched, so an invalid, unstored hash can be downloaded and revalidated from distinct announcers. Confirm this DoS-resistance tradeoff is acceptable.
- [SUGGESTION] commit c89ec5272c9: Squash “fix: apply review suggestions” into the commits it corrects. It only patches unshipped code and tests introduced earlier in this PR, so preserving it would add review-context-dependent bisect/blame noise.
| bool PeerManagerImpl::ConsumeObjectRequest(NodeId nodeid, const CInv& inv) | ||
| { | ||
| AssertLockHeld(cs_main); | ||
|
|
||
| CNodeState* state = State(nodeid); | ||
| if (state == nullptr) | ||
| return false; | ||
|
|
||
| LogPrint(BCLog::NET, "%s -- inv=(%s)\n", __func__, inv.ToString()); | ||
| auto& object_download = state->m_object_download; | ||
| const bool announced = object_download.m_object_announced.erase(inv) != 0; | ||
| const bool in_flight = object_download.m_object_in_flight.erase(inv) != 0; | ||
| if (announced || in_flight) { | ||
| // Also drop any queued (not-yet-sent) GETDATA for this inv from this peer. Unlike | ||
| // EraseObjectRequest we intentionally do not set g_erased_object_requests (to avoid | ||
| // an unsolicited push poisoning the global marker), so a leftover m_object_process_time | ||
| // entry would otherwise make SendMessages issue a redundant GETDATA back to this peer. | ||
| auto& process_time = object_download.m_object_process_time; | ||
| for (auto it = process_time.begin(); it != process_time.end();) { | ||
| const bool match{it->second.type == inv.type && it->second.hash == inv.hash}; | ||
| it = match ? process_time.erase(it) : std::next(it); | ||
| } | ||
| } | ||
| return announced || in_flight; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Squash 'fix: apply review suggestions' into the commits it corrects
Commit c89ec5272c9 ('fix: apply review suggestions') is a review-follow-up whose entire diff corrects code introduced earlier in this same PR, none of which has ever shipped to develop:
- It renames PeerRequestedObject -> PeerConsumeObjectRequest and RequestedObject -> ConsumeObjectRequest, symbols introduced in commit 94d976989b2 (this PR).
- It fixes a real bug in ConsumeObjectRequest (also erase the queued m_object_process_time entry so SendMessages does not issue a redundant GETDATA), in a function likewise introduced in 94d976989b2.
- It updates net_tests.cpp / governance_inv_tests.cpp, both added in commit 3dd5d721e96 (this PR).
Dash merges without squashing, so a generic 'fix: apply review suggestions' commit lands permanently and becomes bisect/blame noise: git blame on the renamed function and the process_time fix would point here instead of at the commit that introduced the logic, and the subject only makes sense alongside the now-gone review conversation. Per the project commit-hygiene guidance (atomic commits, each building and making sense on its own), fold the net_processing.cpp/.h and net_governance.cpp changes into 94d976989b2 and the test changes into 3dd5d721e96 (e.g. git rebase -i --autosquash after marking them fixup!). Non-blocking, but the stack should be cleaned before merge.
source: ['claude']
… tracker Governance tracked pending object/vote requests in a global, governance-owned cache (CGovernanceManager::m_requested_hash_time): ConfirmInventoryRequest recorded a hash and AcceptMessage authorized the eventual response, expiring entries after RELIABLE_PROPAGATION_TIME. The cache was unbounded, and a global bound would let a single INV saturate it. The net layer already tracks pending object requests per peer in CNodeState::ObjectDownloadState (m_object_announced / m_object_in_flight), which is bounded per peer by construction, and governance already touches it on receipt. Reuse it as the authorization source and drop the governance-side cache: - Add PeerManager::PeerConsumeObjectRequest(nodeid, inv): consume (erase) the peer's per-peer announced / in-flight state and return whether such a request existed (the peer announced the inv, or we requested it from the peer). It deliberately does NOT set the global g_already_asked_for / g_erased_object_requests markers, so an unsolicited push from an untracked peer cannot poison request scheduling and suppress a later legitimate fetch. PeerEraseObjectRequest / EraseObjectRequest keep their existing behavior for tx relay and the other subsystems. - SendMessages skips draining a queued request whose per-peer announced/in-flight state was already consumed (or received), so consuming an announcement never triggers a redundant GETDATA and stays O(1) (no scan of m_object_process_time). - Gate MNGOVERNANCEOBJECT / MNGOVERNANCEOBJECTVOTE acceptance on PeerConsumeObjectRequest instead of AcceptMessage. - Remove m_requested_hash_time, AcceptMessage, the ConfirmInventoryRequest cache write, the CheckAndRemove prune, and the obsolete request-cache unit tests. ConfirmInventoryRequest keeps only its "do we already have it?" role for AlreadyHave. Acceptance becomes per-peer: an object/vote is accepted only if the sending peer announced it or we requested it from that peer. In the inv->getdata gossip model senders are always prior announcers, so legitimate flows are unaffected; genuinely unsolicited pushes are now rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- net_tests: peer_consume_object_request_authorizes_and_erases_per_peer_state exercises PeerConsumeObjectRequest for announced-only and requested invs, that it erases the per-peer entry, that a consumed announcement is not re-requested by SendMessages (drain-side skip, no redundant GETDATA), and the anti-poisoning property (a rejected unsolicited push must not suppress a later legitimate fetch). - governance_inv_tests: object and vote acceptance require the sending peer to have announced the inv (or us to have requested it); an unsolicited peer is rejected without misbehaving, and a second independent announcer's copy is accepted with its own per-peer entry consumed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c89ec52 to
0c0ec8c
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: orchestrator=openai/gpt-5.6-sol(high, orchestration-only); reviewers codex/general=gpt-5.6-sol(completed), sonnet5/general=claude-sonnet-5(completed), codex/dash-core-commit-history=gpt-5.6-sol(completed), sonnet5/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude-sonnet-5 (verifier-sonnet5-7442-1783702223).
Prior Finding Reconciliation
- STILL VALID — Invalid governance objects announced by multiple peers are no longer suppressed from re-download. Both general reviewers and the primary verifier confirmed that
ConsumeObjectRequeststill erases only per-peer state; the new drain-side check prevents a redundant GETDATA to that peer but does not suppress another peer’s copy of the same invalid, unstored hash. - FIXED — Squash
fix: apply review suggestionsinto the commits it corrects. The branch was rewritten into two clean commits (3f5536cae6fimplementation,0c0ec8c4b74tests); the review-fix commit is no longer reachable and its content was folded into those commits.
Carried-Forward Prior Findings
One non-blocking suggestion remains: maintainers should explicitly accept the cross-peer invalid-object re-download/revalidation tradeoff or use a distinct known-invalid marker after conclusive local validation failure.
New Findings In Latest Delta
None. Moving authorization consumption after the not-synced gate is consistent for objects and votes and is covered by the new retransmit test.
Additional Cumulative Findings
None. Both commit-history lanes independently found the rewritten two-commit stack clean.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/net_processing.cpp`:
- [SUGGESTION] src/net_processing.cpp:1592-1609: Invalid governance objects announced by multiple peers are no longer suppressed from re-download
ConsumeObjectRequest (net_processing.cpp:1592-1609) erases only the per-peer m_object_announced/m_object_in_flight state and, per its own comment at 1604-1608, deliberately never sets the global g_erased_object_requests marker — this is intentional, to stop an unsolicited push from one peer from poisoning global scheduling and suppressing a later legitimate fetch from another peer. The SendMessages drain-side check (net_processing.cpp:6694-6701) only inspects the draining peer's own announced/in-flight state before falling through to the g_erased_object_requests check, so it cannot suppress a *different* peer's still-queued process_time entry for the same hash.
For objects that end up stored this is harmless (AlreadyHave suppresses re-download for everyone). But when NetGovernance::ProcessMessage rejects a genuinely invalid object (governance/net_governance.cpp:191-193, ProcessObject returns false and the peer gets a +20 penalty), the same invalid hash announced by other peers is still re-requested, re-downloaded over a full getdata round-trip, and re-validated, since no global suppression marker is ever set for it. This is self-limiting per peer (ban at 100 misbehavior points) and is an explicit, well-documented tradeoff to avoid the poisoning risk on the other side — not a correctness bug. Flagging so maintainers can explicitly confirm the DoS-resistance tradeoff (repeated re-validation cost of a widely-gossiped invalid object across N peers) is acceptable as-is, or consider recording a lightweight per-hash 'known-invalid' marker only after local validation has conclusively failed (distinct from the removed unbounded governance-owned request cache).
|
Follow-up cleanup audit at
I found no stale forward declarations and no dangling references to |
knst
left a comment
There was a problem hiding this comment.
see #7442 (comment) claw's comment about cleaning up unused includes
my nit about CNode.
Otherwise it's LGTM
| auto peer{std::make_unique<CNode>(id, | ||
| /*sock=*/nullptr, | ||
| /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, | ||
| /*nKeyedNetGroupIn=*/0, | ||
| /*nLocalHostNonceIn=*/0, | ||
| /*addrBindIn=*/CAddress{}, | ||
| /*addrNameIn=*/std::string{}, | ||
| /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, | ||
| /*inbound_onion=*/false)}; |
There was a problem hiding this comment.
nit: can you make follow-up changes to use this helper inside per_object_vote_sync_is_fulfilled_request_limited to avoid duplicated code for creating CNode?
There was a problem hiding this comment.
Addressed in follow-up #7449 by reusing the existing governance-inv peer helper; it also includes the cleanup audit referenced in the review body.
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in #7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0
…che removal 2673240 test: extract AssertMisbehaviorScore helper in governance inv tests (PastaClaw) a8f2711 refactor(governance): move RELIABLE_PROPAGATION_TIME next to its only uses (PastaClaw) 83d8dd7 test: drop the governance inv fixture setup left over from CheckAndRemove (PastaClaw) 97abde6 test: reuse the peer helper in per_object_vote_sync_is_fulfilled_request_limited (PastaClaw) Pull request description: ## What was done? Follow-up to #7442 and knst's review: - reuse the existing `MakeGovernanceInvPeer` helper in `per_object_vote_sync_is_fulfilled_request_limited` - remove the stale scheduler/thread/meta-manager fixture setup left behind when the `CheckAndRemove` tests were deleted - update the fixture comments to describe the remaining sync and mock-time requirements - extract an `AssertMisbehaviorScore` helper to remove repetitive state-stat assertions - move `RELIABLE_PROPAGATION_TIME` beside its only remaining uses in `governance.cpp` and document its current additional-relay role Review context: #7442 (review) ## How was this tested? - full headless build - `test_dash --run_test=governance_inv_tests,net_tests,ratecheck_tests,miner_tests` - `lint-whitespace.py` - `lint-include-guards.py` - `lint-includes.py` - `lint-circular-dependencies.py` - `git diff --check` The mandatory independent pre-PR code-review gate returned `ship` with no findings. ACKs for top commit: UdjinM6: utACK 2673240 Tree-SHA512: 5e4ef12b85f355b130e90c305f53b5b58aef8708cb15f58f40afa2bcfdfac548a3784b8fc45a687df96de0b399cfa80137b75cde21aeaa213af03aff6e799b49
Backport prerequisite from dashpay#7387: expose RequestedHashCacheSizeForTesting() and governance::RELIABLE_PROPAGATION_TIME so the v23.1.x unit-test migration of governance inv cache coverage can observe ConfirmInventoryRequest / CheckAndRemove expiration without scraping logs. The full dashpay#7387 merge is intentionally not cherry-picked: v23.1.8 retains p2p_governance_invs.py and only needs the unit-test fixture subset required by later dashpay#7414/dashpay#7442 coverage.
Backport prerequisite for dashpay#7414/dashpay#7442: add the minimal governance inventory unit-test fixture and NetGovernance::Schedule CheckAndRemove coverage that dashpay#7387 introduced, without removing p2p_governance_invs.py from v23.1.x. Covers ConfirmInventoryRequest request-cache expiration and the periodic Schedule cleanup path used by later governance throttle/authorization tests.
…the net-layer per-peer request tracker 0c0ec8c test: cover net-layer per-peer governance inv authorization (UdjinM6) 3f5536c fix(net): authorize governance inv responses via the per-peer request tracker (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Governance tracked pending object/vote requests in a global, governance-owned cache (`CGovernanceManager::m_requested_hash_time`). `ConfirmInventoryRequest` recorded a hash when we decided to fetch an inv, and `AcceptMessage` authorized the eventual response (accepting once, then erasing), with entries expiring after `RELIABLE_PROPAGATION_TIME`. That cache was unbounded. ~Bounding it with a global cap (as in dashpay#7425) is only a partial fix: the cap has to be large, and because it is global, a single peer can fill it (a `MNGOVERNANCESYNC`/INV burst) and stall governance-inv intake for everyone until entries expire.~ 18d4725d99cfe527c859621932ed48b436517df4 fixed that. The net layer already tracks pending object requests **per peer** in `CNodeState::ObjectDownloadState` (`m_object_announced` / `m_object_in_flight`), which is bounded per peer by construction (`MAX_PEER_OBJECT_ANNOUNCEMENTS` / `MAX_PEER_OBJECT_IN_FLIGHT`), and governance already touches it on receipt (`PeerEraseObjectRequest`). The two trackers are populated in lockstep — the same `AlreadyHave` call that populated `m_requested_hash_time` also drove `RequestObject`. So the governance-side cache is redundant with state the net layer already keeps, and the net copy cannot be flooded by one peer. This reuses the net-layer tracker as the authorization source and removes the governance-side cache, eliminating the flood surface structurally instead of bounding it. ## What was done? - Add `PeerManager::PeerRequestedObject(nodeid, inv)`: erases **only** the per-peer `m_object_announced` / `m_object_in_flight` for that peer and returns whether the inv was tracked (announced by, or requested from, that peer). It deliberately does **not** touch the global `g_already_asked_for` / `g_erased_object_requests` markers, so an unsolicited push of a known hash from an untracked peer cannot poison request scheduling and suppress a later legitimate fetch. `PeerEraseObjectRequest` / `EraseObjectRequest` keep their existing behavior and callers (tx relay and the other Dash subsystems) unchanged. - Gate `MNGOVERNANCEOBJECT` / `MNGOVERNANCEOBJECTVOTE` acceptance on `PeerRequestedObject` instead of `AcceptMessage`. - Remove `m_requested_hash_time`, `AcceptMessage`, the `ConfirmInventoryRequest` cache write, the `CheckAndRemove` prune, and the obsolete request-cache unit tests. `ConfirmInventoryRequest` keeps only its "do we already have it?" role for `AlreadyHave`. Acceptance becomes per-peer: an object/vote is accepted only if the sending peer announced it or we requested it from that peer. In the inv → getdata gossip model a sender is always a prior announcer, and governance objects/votes are only ever sent as `getdata` responses (never pushed unsolicited), so legitimate flows are unaffected; genuinely unsolicited pushes are now rejected. This is the same INV-tracking requirement the old `AcceptMessage` path already had (`m_requested_hash_time` was only populated on an INV), so no legitimately-served response is newly rejected. Tests covering the modified code: - `src/test/net_tests.cpp` — `peer_requested_object_authorizes_and_erases_per_peer_state`: exercises `PeerRequestedObject` for announced-only and in-flight invs, that it erases the per-peer entry, and the anti-poisoning property (a rejected unsolicited push must not set `g_erased_object_requests` and suppress a later legitimate fetch — driven through the real `SendMessages` getdata scheduler). - `src/test/governance_inv_tests.cpp` — `governance_objects_require_peer_announcement_or_request` and `governance_votes_require_peer_announcement_or_request`: an announcing peer is accepted (and the gate consumes its per-peer tracker entry), an unsolicited peer is rejected without misbehaving, and a second independent announcer's duplicate is accepted idempotently. ## How Has This Been Tested? macOS, `--enable-debug` build. - `make -C src test/test_dash -j"$(sysctl -n hw.ncpu)"` — clean build; each commit builds on its own. - `src/test/test_dash --run_test=net_tests` (19 cases) — pass, no regression from restoring the request-erase functions. - `src/test/test_dash --run_test=governance_inv_tests` (3 cases) — pass. - `src/test/test_dash --run_test=denialofservice_tests` (5 cases) — pass (object-request machinery). - `git diff --check` and `test/lint/lint-whitespace.py` — clean. Follow-up (not in this PR): a functional test driving an end-to-end multi-node governance sync plus the unsolicited-push-does-not-suppress-fetch case. ## Breaking Changes None. No consensus, protocol-message, or serialization change. The only behavioral change is that a governance object/vote pushed by a peer that neither announced it nor was asked for it is now rejected — stricter anti-unsolicited handling on a best-effort, non-consensus gossip path. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 0c0ec8c Tree-SHA512: d93ebf0366e53cdba61cfeb495159e274b9b940d459ae6e4c5f370358d828835fac4d2da3c7765b3c72634980f549eb1bcb7dadcb3e98ebe63219c90b3d634f1
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0
After dashpay#7442, governance inv authorization no longer uses m_requested_hash_time / ConfirmInventoryRequest cache writes. Pending object and vote requests live in the net-layer per-peer tracker (m_object_announced / m_object_in_flight). Rewrite the functional test to assert the current behavior: - a new govobj/govobjvote INV is queued via RequestObject and fetched - a same-peer duplicate is not re-queued while the announcement is live - governance UpdateCachesAndClean does not clear that net-layer state - after in-flight expiry, the same INV is accepted and requested again
Narrow exception from Dash dashpay#7387 for the v23.1.8 release history. The full dashpay#7387 merge removes p2p_governance_invs.py after migrating coverage to unit tests. v23.1.x still ships that functional test, so only the prerequisite fixture/coverage subset is included here: - expose RequestedHashCacheSizeForTesting() and governance::RELIABLE_PROPAGATION_TIME for unit observation of ConfirmInventoryRequest / CheckAndRemove - add governance_inv_tests coverage for request-cache expiration and NetGovernance::Schedule periodic cleanup Later dashpay#7414/dashpay#7442 build on these fixtures. p2p_governance_invs.py is retained and updated with dashpay#7442. Co-Authored-By: Claude <noreply@anthropic.com>
…the net-layer per-peer request tracker 0c0ec8c test: cover net-layer per-peer governance inv authorization (UdjinM6) 3f5536c fix(net): authorize governance inv responses via the per-peer request tracker (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Governance tracked pending object/vote requests in a global, governance-owned cache (`CGovernanceManager::m_requested_hash_time`). `ConfirmInventoryRequest` recorded a hash when we decided to fetch an inv, and `AcceptMessage` authorized the eventual response (accepting once, then erasing), with entries expiring after `RELIABLE_PROPAGATION_TIME`. That cache was unbounded. ~Bounding it with a global cap (as in dashpay#7425) is only a partial fix: the cap has to be large, and because it is global, a single peer can fill it (a `MNGOVERNANCESYNC`/INV burst) and stall governance-inv intake for everyone until entries expire.~ 18d4725d99cfe527c859621932ed48b436517df4 fixed that. The net layer already tracks pending object requests **per peer** in `CNodeState::ObjectDownloadState` (`m_object_announced` / `m_object_in_flight`), which is bounded per peer by construction (`MAX_PEER_OBJECT_ANNOUNCEMENTS` / `MAX_PEER_OBJECT_IN_FLIGHT`), and governance already touches it on receipt (`PeerEraseObjectRequest`). The two trackers are populated in lockstep — the same `AlreadyHave` call that populated `m_requested_hash_time` also drove `RequestObject`. So the governance-side cache is redundant with state the net layer already keeps, and the net copy cannot be flooded by one peer. This reuses the net-layer tracker as the authorization source and removes the governance-side cache, eliminating the flood surface structurally instead of bounding it. ## What was done? - Add `PeerManager::PeerRequestedObject(nodeid, inv)`: erases **only** the per-peer `m_object_announced` / `m_object_in_flight` for that peer and returns whether the inv was tracked (announced by, or requested from, that peer). It deliberately does **not** touch the global `g_already_asked_for` / `g_erased_object_requests` markers, so an unsolicited push of a known hash from an untracked peer cannot poison request scheduling and suppress a later legitimate fetch. `PeerEraseObjectRequest` / `EraseObjectRequest` keep their existing behavior and callers (tx relay and the other Dash subsystems) unchanged. - Gate `MNGOVERNANCEOBJECT` / `MNGOVERNANCEOBJECTVOTE` acceptance on `PeerRequestedObject` instead of `AcceptMessage`. - Remove `m_requested_hash_time`, `AcceptMessage`, the `ConfirmInventoryRequest` cache write, the `CheckAndRemove` prune, and the obsolete request-cache unit tests. `ConfirmInventoryRequest` keeps only its "do we already have it?" role for `AlreadyHave`. Acceptance becomes per-peer: an object/vote is accepted only if the sending peer announced it or we requested it from that peer. In the inv → getdata gossip model a sender is always a prior announcer, and governance objects/votes are only ever sent as `getdata` responses (never pushed unsolicited), so legitimate flows are unaffected; genuinely unsolicited pushes are now rejected. This is the same INV-tracking requirement the old `AcceptMessage` path already had (`m_requested_hash_time` was only populated on an INV), so no legitimately-served response is newly rejected. Tests covering the modified code: - `src/test/net_tests.cpp` — `peer_requested_object_authorizes_and_erases_per_peer_state`: exercises `PeerRequestedObject` for announced-only and in-flight invs, that it erases the per-peer entry, and the anti-poisoning property (a rejected unsolicited push must not set `g_erased_object_requests` and suppress a later legitimate fetch — driven through the real `SendMessages` getdata scheduler). - `src/test/governance_inv_tests.cpp` — `governance_objects_require_peer_announcement_or_request` and `governance_votes_require_peer_announcement_or_request`: an announcing peer is accepted (and the gate consumes its per-peer tracker entry), an unsolicited peer is rejected without misbehaving, and a second independent announcer's duplicate is accepted idempotently. ## How Has This Been Tested? macOS, `--enable-debug` build. - `make -C src test/test_dash -j"$(sysctl -n hw.ncpu)"` — clean build; each commit builds on its own. - `src/test/test_dash --run_test=net_tests` (19 cases) — pass, no regression from restoring the request-erase functions. - `src/test/test_dash --run_test=governance_inv_tests` (3 cases) — pass. - `src/test/test_dash --run_test=denialofservice_tests` (5 cases) — pass (object-request machinery). - `git diff --check` and `test/lint/lint-whitespace.py` — clean. Follow-up (not in this PR): a functional test driving an end-to-end multi-node governance sync plus the unsolicited-push-does-not-suppress-fetch case. ## Breaking Changes None. No consensus, protocol-message, or serialization change. The only behavioral change is that a governance object/vote pushed by a peer that neither announced it nor was asked for it is now rejected — stricter anti-unsolicited handling on a best-effort, non-consensus gossip path. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 0c0ec8c Tree-SHA512: d93ebf0366e53cdba61cfeb495159e274b9b940d459ae6e4c5f370358d828835fac4d2da3c7765b3c72634980f549eb1bcb7dadcb3e98ebe63219c90b3d634f1
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0
…lization 8f0b813 fix(governance): bound vote signature deserialization (PastaClaw) Pull request description: Uses the shared bounded-vector deserialization primitive merged in dashpay#7439. ## Motivation Governance vote signatures were deserialized through the generic byte-vector path. A peer could declare a very large signature length, causing allocation before the stream reported truncation. The outer message-processing catch did not score or disconnect the peer, allowing repeated malformed messages. ## Changes - bound network governance-vote signature reads to 96 bytes before allocation - require one of the two structurally valid encodings: 65-byte compact ECDSA or 96-byte BLS - score malformed or truncated governance vote messages with 100 misbehavior points - preserve disk, hash, and outbound serialization behavior - add focused unit coverage ## Testing - `./src/test/test_dash --run_test=governance_vote_wire_tests` (4/4 tests) - `./src/test/test_dash --run_test=serialize_tests` (10/10 tests) - `test/lint/lint-python.py` Tree-SHA512: backported to v23.1.x by cherry-picking 8f0b813 (applies cleanly). Backport note for v23.1.8 ------------------------- This was missing from the original v23.1.8 branch while its test follow-up dashpay#7450 ("test: make governance vote fixtures wire-valid", 915566d) was already included. That ordering was inverted: dashpay#7450 exists solely to adapt the dashpay#7442 governance-inv fixtures to the bound that dashpay#7440 introduces. Verified by removing dashpay#7450's SetSignature() line and rebuilding: without dashpay#7440 present the fixtures pass regardless, and re-adding dashpay#7440 reproduces exactly the six governance_inv_tests failures dashpay#7450's description cites. So the branch was shipping the compensating test change for a hardening fix it did not have, leaving CGovernanceVote::vchSig unbounded on the network path. The prerequisite dashpay#7439 (LIMITED_VECTOR) is already present via 099b99d, as are the sibling bounding backports dashpay#7416/dashpay#7418/dashpay#7419/dashpay#7438/dashpay#7444, so this restores the intended set rather than widening release scope. Reported-by: UdjinM6 Co-Authored-By: Claude <noreply@anthropic.com>
…it tests Backport of dashpay#7387 (upstream merge da52293, cherry-picked with -m1). Included because dashpay#7414/dashpay#7442/dashpay#7450 modify src/test/governance_inv_tests.cpp, which this PR introduces; it also replaces the p2p_governance_invs.py functional test with equivalent unit-test coverage. v23.1.x adaptations: (1) governance.h on this branch has no 'namespace governance' block at the top - added one containing only the RELIABLE_PROPAGATION_TIME constant this PR introduces (develop's SuperblockManager forward declaration does not exist here and was not brought along). (2) ProcessVoteAndRelay keeps this branch's 'override' specifier. (3) Makefile.test.include entry added without develop-only governance_superblock_tests.cpp. All other hunks unchanged from upstream. (cherry picked from commit da52293e8d6ba51fca27a557f6efd83a439a4b70)
…the net-layer per-peer request tracker Backport of dashpay#7442 (upstream merge 13b9071, cherry-picked with -m1). v23.1.x adaptations, all dropping develop-only context rather than changing the fix: (1) develop registers NetGovernance unconditionally and implements AlreadyHave/ProcessGetData on it (both from out-of-scope commit 58ab8b3); this branch keeps its conditional registration and answers governance invs inline in net_processing (which already consults ConfirmInventoryRequest), so those context blocks were not brought along. (2) PeerManagerInternal here has no PeerPushInventory - the adjacent context line was not added. (3) ProcessVoteAndRelay keeps this branch's override specifier and ProcessObject keeps this branch's CNode& parameter. The substantive changes - ConsumeObjectRequest/PeerConsumeObjectRequest, the SendMessages stale-entry drain, removal of the governance-side m_requested_hash_time cache/AcceptMessage, the PeerConsumeObjectRequest authorization gates in NetGovernance::ProcessMessage, and both test files - are unchanged from upstream. (cherry picked from commit 13b9071880b0e995ba7a1233be2b95dd7e7c56cf)
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0 (cherry picked from commit f343d58)
…the net-layer per-peer request tracker Backport of dashpay#7442 (upstream merge 13b9071, cherry-picked with -m1). v23.1.x adaptations, all dropping develop-only context rather than changing the fix: (1) develop registers NetGovernance unconditionally and implements AlreadyHave/ProcessGetData on it (both from out-of-scope commit 58ab8b3); this branch keeps its conditional registration and answers governance invs inline in net_processing (which already consults ConfirmInventoryRequest), so those context blocks were not brought along. (2) PeerManagerInternal here has no PeerPushInventory - the adjacent context line was not added. (3) ProcessVoteAndRelay keeps this branch's override specifier and ProcessObject keeps this branch's CNode& parameter. The substantive changes - ConsumeObjectRequest/PeerConsumeObjectRequest, the SendMessages stale-entry drain, removal of the governance-side m_requested_hash_time cache/AcceptMessage, the PeerConsumeObjectRequest authorization gates in NetGovernance::ProcessMessage, and both test files - are unchanged from upstream. (cherry picked from commit 13b9071880b0e995ba7a1233be2b95dd7e7c56cf)
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0 (cherry picked from commit f343d58)
…the net-layer per-peer request tracker Backport of dashpay#7442 (upstream merge 13b9071, cherry-picked with -m1). v23.1.x adaptations, all dropping develop-only context rather than changing the fix: (1) develop registers NetGovernance unconditionally and implements AlreadyHave/ProcessGetData on it (both from out-of-scope commit 58ab8b3); this branch keeps its conditional registration and answers governance invs inline in net_processing (which already consults ConfirmInventoryRequest), so those context blocks were not brought along. (2) PeerManagerInternal here has no PeerPushInventory - the adjacent context line was not added. (3) ProcessVoteAndRelay keeps this branch's override specifier and ProcessObject keeps this branch's CNode& parameter. The substantive changes - ConsumeObjectRequest/PeerConsumeObjectRequest, the SendMessages stale-entry drain, removal of the governance-side m_requested_hash_time cache/AcceptMessage, the PeerConsumeObjectRequest authorization gates in NetGovernance::ProcessMessage, and both test files - are unchanged from upstream. (cherry picked from commit 13b9071880b0e995ba7a1233be2b95dd7e7c56cf)
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0 (cherry picked from commit f343d58)
…the net-layer per-peer request tracker Backport of dashpay#7442 (upstream merge 13b9071, cherry-picked with -m1). v23.1.x adaptations, all dropping develop-only context rather than changing the fix: (1) develop registers NetGovernance unconditionally and implements AlreadyHave/ProcessGetData on it (both from out-of-scope commit 58ab8b3); this branch keeps its conditional registration and answers governance invs inline in net_processing (which already consults ConfirmInventoryRequest), so those context blocks were not brought along. (2) PeerManagerInternal here has no PeerPushInventory - the adjacent context line was not added. (3) ProcessVoteAndRelay keeps this branch's override specifier and ProcessObject keeps this branch's CNode& parameter. The substantive changes - ConsumeObjectRequest/PeerConsumeObjectRequest, the SendMessages stale-entry drain, removal of the governance-side m_requested_hash_time cache/AcceptMessage, the PeerConsumeObjectRequest authorization gates in NetGovernance::ProcessMessage, and both test files - are unchanged from upstream. (cherry picked from commit 13b9071880b0e995ba7a1233be2b95dd7e7c56cf)
e058152 test: make governance vote fixtures wire-valid (PastaClaw) Pull request description: ## Issue being fixed or feature implemented The governance inventory tests merged in dashpay#7442 construct unsigned synthetic votes and round-trip them through the network parser. Develop now requires governance vote signatures to use a structurally valid 65- or 96-byte encoding, so the fixture is rejected as malformed before the authorization behavior under test is reached. This causes six unit-test failures across CI configurations. ## What was done? Give the synthetic `VOTE_SIGNAL_FUNDING` vote a compact-signature-sized placeholder. The 65-byte encoding matches the real ECDSA voting-key form for funding votes. These tests deliberately use a missing parent object, so vote processing takes the orphan path before cryptographic signature verification; production validation is unchanged. ## How Has This Been Tested? - Built `src/test/test_dash` in an isolated macOS arm64 worktree using the depends toolchain - `./src/test/test_dash --run_test=governance_inv_tests` - `./src/test/test_dash --run_test=governance_vote_wire_tests` - `git diff --check` - Mandatory independent pre-PR review gate: `ship` with no findings ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK e058152 Tree-SHA512: 5caca1e34ce563444ded42f669ff599f7658c6d1dbea1d541588211ac542ee45f3dd768e158899cb23150c5d45ec8dbcbf412a1472f73a3cc3e72a5ac37636e0 (cherry picked from commit f343d58)
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
Issue being fixed or feature implemented
Governance tracked pending object/vote requests in a global, governance-owned cache
(
CGovernanceManager::m_requested_hash_time).ConfirmInventoryRequestrecorded a hash when wedecided to fetch an inv, and
AcceptMessageauthorized the eventual response (accepting once, thenerasing), with entries expiring after
RELIABLE_PROPAGATION_TIME.That cache was unbounded.
Bounding it with a global cap (as in #7425) is only a partial fix: the18d4725d99cfe527c859621932ed48b436517df4 fixed that.cap has to be large, and because it is global, a single peer can fill it (a
MNGOVERNANCESYNC/INVburst) and stall governance-inv intake for everyone until entries expire.
The net layer already tracks pending object requests per peer in
CNodeState::ObjectDownloadState(m_object_announced/m_object_in_flight), which is boundedper peer by construction (
MAX_PEER_OBJECT_ANNOUNCEMENTS/MAX_PEER_OBJECT_IN_FLIGHT), andgovernance already touches it on receipt (
PeerEraseObjectRequest). The two trackers are populatedin lockstep — the same
AlreadyHavecall that populatedm_requested_hash_timealso droveRequestObject. So the governance-side cache is redundant with state the net layer already keeps,and the net copy cannot be flooded by one peer.
This reuses the net-layer tracker as the authorization source and removes the governance-side cache,
eliminating the flood surface structurally instead of bounding it.
What was done?
PeerManager::PeerRequestedObject(nodeid, inv): erases only the per-peerm_object_announced/m_object_in_flightfor that peer and returns whether the inv was tracked(announced by, or requested from, that peer). It deliberately does not touch the global
g_already_asked_for/g_erased_object_requestsmarkers, so an unsolicited push of a known hashfrom an untracked peer cannot poison request scheduling and suppress a later legitimate fetch.
PeerEraseObjectRequest/EraseObjectRequestkeep their existing behavior and callers (tx relayand the other Dash subsystems) unchanged.
MNGOVERNANCEOBJECT/MNGOVERNANCEOBJECTVOTEacceptance onPeerRequestedObjectinstead ofAcceptMessage.m_requested_hash_time,AcceptMessage, theConfirmInventoryRequestcache write, theCheckAndRemoveprune, and the obsolete request-cache unit tests.ConfirmInventoryRequestkeepsonly its "do we already have it?" role for
AlreadyHave.Acceptance becomes per-peer: an object/vote is accepted only if the sending peer announced it or we
requested it from that peer. In the inv → getdata gossip model a sender is always a prior announcer,
and governance objects/votes are only ever sent as
getdataresponses (never pushed unsolicited), solegitimate flows are unaffected; genuinely unsolicited pushes are now rejected. This is the same
INV-tracking requirement the old
AcceptMessagepath already had (m_requested_hash_timewas onlypopulated on an INV), so no legitimately-served response is newly rejected.
Tests covering the modified code:
src/test/net_tests.cpp—peer_requested_object_authorizes_and_erases_per_peer_state: exercisesPeerRequestedObjectfor announced-only and in-flight invs, that it erases the per-peer entry, andthe anti-poisoning property (a rejected unsolicited push must not set
g_erased_object_requestsandsuppress a later legitimate fetch — driven through the real
SendMessagesgetdata scheduler).src/test/governance_inv_tests.cpp—governance_objects_require_peer_announcement_or_requestandgovernance_votes_require_peer_announcement_or_request: an announcing peer is accepted (and thegate consumes its per-peer tracker entry), an unsolicited peer is rejected without misbehaving, and a
second independent announcer's duplicate is accepted idempotently.
How Has This Been Tested?
macOS,
--enable-debugbuild.make -C src test/test_dash -j"$(sysctl -n hw.ncpu)"— clean build; each commit builds on its own.src/test/test_dash --run_test=net_tests(19 cases) — pass, no regression from restoring therequest-erase functions.
src/test/test_dash --run_test=governance_inv_tests(3 cases) — pass.src/test/test_dash --run_test=denialofservice_tests(5 cases) — pass (object-request machinery).git diff --checkandtest/lint/lint-whitespace.py— clean.Follow-up (not in this PR): a functional test driving an end-to-end multi-node governance sync plus
the unsolicited-push-does-not-suppress-fetch case.
Breaking Changes
None. No consensus, protocol-message, or serialization change. The only behavioral change is that a
governance object/vote pushed by a peer that neither announced it nor was asked for it is now
rejected — stricter anti-unsolicited handling on a best-effort, non-consensus gossip path.
Checklist: